Skip to content

Make perm(::Vector{Int}) constructor and application of permutations to integers faster and allocate less #5476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make perm(::Vector{Int}) constructor and application of permutations to integers faster and allocate less #5476

wants to merge 1 commit into from

Conversation

@fingolfin
Copy link
Member

@fingolfin fingolfin commented Oct 20, 2025

Before:

julia> make_test_data(n) = (symmetric_group(n), vcat([n],1:n-1));

julia> testit(pi) = [ i^pi for i in 1:degree(pi) ];

julia> G, L = make_test_data(1000); pi = G(L); @b testit(pi)
8.222 μs (492 allocs: 15.703 KiB)

julia> G, L = make_test_data(10000); pi = G(L); @b testit(pi)
98.916 μs (9492 allocs: 244.328 KiB)

julia> G, L = make_test_data(100000); pi = G(L); @b testit(pi)
1.069 ms (99492 allocs: 2.299 MiB)

julia> G, L = make_test_data(1000); @b $G($L)
12.834 μs (4 allocs: 10.016 KiB)

julia> G, L = make_test_data(10000); @b $G($L)
124.500 μs (4 allocs: 97.906 KiB)

julia> G, L = make_test_data(100000); @b $G($L)
1.235 ms (4 allocs: 1.145 MiB)

After:

julia> make_test_data(n) = (symmetric_group(n), vcat([n],1:n-1));

julia> testit(pi) = [ i^pi for i in 1:degree(pi) ];

julia> G, L = make_test_data(1000); pi = G(L); @b testit(pi)
1.041 μs (3 allocs: 8.062 KiB)

julia> G, L = make_test_data(10000); pi = G(L); @b testit(pi)
9.833 μs (3 allocs: 96.062 KiB)

julia> G, L = make_test_data(100000); pi = G(L); @b testit(pi)
104.625 μs (3 allocs: 800.062 KiB)

julia> G, L = make_test_data(1000); @b $G($L)
1.905 μs (2 allocs: 2.000 KiB)

julia> G, L = make_test_data(10000); @b $G($L)
16.458 μs (2 allocs: 19.641 KiB)

julia> G, L = make_test_data(100000); @b $G($L)
159.458 μs (2 allocs: 390.766 KiB)

@fingolfin fingolfin added enhancement New feature or request topic: groups package: GAP optimization Simpler/more performant code or more/better tests release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Oct 20, 2025
ThomasBreuer
ThomasBreuer approved these changes Oct 20, 2025
View reviewed changes
Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Just for curiosity:
For n of type T different from Int, now we compute n^g by first converting n to Int, then computing the image of that Int, and then converting the result to T.
Is this cheaper than converting the unsafe_load result directly to T?

lgoettgens
lgoettgens reviewed Oct 20, 2025
View reviewed changes
Copy link
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests fail

@fingolfin fingolfin force-pushed the mh/permutation-optimization branch from 7a03e93 to a883365 Compare October 24, 2025 12:21
lgoettgens
lgoettgens reviewed Oct 27, 2025
View reviewed changes
lgoettgens
lgoettgens reviewed Oct 27, 2025
View reviewed changes
lgoettgens
lgoettgens reviewed Oct 27, 2025
View reviewed changes
src/Groups/perm.jl
return x
end

function _make_gap_perm(::Type{T}, deg::Int, L::AbstractVector{<:IntegerUnion}) where {T <: Union{UInt16, UInt32}}
Copy link
Member

@lgoettgens lgoettgens Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this function should belong to Oscar or rather to GAP.jl as it contains ccalls into libgap. That seems to be an implementation detail of GAP.jl. But no strong opinion on this.

Copy link
Member Author

@fingolfin fingolfin Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the long run, yes, maybe it should go to GAP.jl. But I'd like to first have something working, and I expect a few similar changes and tweaks to be needed first.

@fingolfin fingolfin force-pushed the mh/permutation-optimization branch from 734b705 to 0625c46 Compare November 5, 2025 22:42
@fingolfin
Faster perm(::Vector{Int}) & application of permutations
ac6e83e 
Before:

    julia> make_test_data(n) = (symmetric_group(n), vcat([n],1:n-1));

    julia> testit(pi) = [ i^pi for i in 1:degree(pi) ];

    julia> G, L = make_test_data(1000); pi = G(L); @b testit(pi)
    8.222 μs (492 allocs: 15.703 KiB)

    julia> G, L = make_test_data(10000); pi = G(L); @b testit(pi)
    98.916 μs (9492 allocs: 244.328 KiB)

    julia> G, L = make_test_data(100000); pi = G(L); @b testit(pi)
    1.069 ms (99492 allocs: 2.299 MiB)

    julia> G, L = make_test_data(1000); @b $G($L)
    12.834 μs (4 allocs: 10.016 KiB)

    julia> G, L = make_test_data(10000); @b $G($L)
    124.500 μs (4 allocs: 97.906 KiB)

    julia> G, L = make_test_data(100000); @b $G($L)
    1.235 ms (4 allocs: 1.145 MiB)

After:

    julia> make_test_data(n) = (symmetric_group(n), vcat([n],1:n-1));

    julia> testit(pi) = [ i^pi for i in 1:degree(pi) ];

    julia> G, L = make_test_data(1000); pi = G(L); @b testit(pi)
    1.041 μs (3 allocs: 8.062 KiB)

    julia> G, L = make_test_data(10000); pi = G(L); @b testit(pi)
    9.833 μs (3 allocs: 96.062 KiB)

    julia> G, L = make_test_data(100000); pi = G(L); @b testit(pi)
    104.625 μs (3 allocs: 800.062 KiB)

    julia> G, L = make_test_data(1000); @b $G($L)
    1.905 μs (2 allocs: 2.000 KiB)

    julia> G, L = make_test_data(10000); @b $G($L)
    16.458 μs (2 allocs: 19.641 KiB)

    julia> G, L = make_test_data(100000); @b $G($L)
    159.458 μs (2 allocs: 390.766 KiB)
@fingolfin fingolfin force-pushed the mh/permutation-optimization branch from 0625c46 to ac6e83e Compare November 5, 2025 22:48
fingolfin
fingolfin commented Nov 5, 2025
View reviewed changes
src/Groups/perm.jl
Comment on lines +360 to +366
for i in 1:length(L)
unsafe_store!(addr, UInt64(L[i]) - 1, i)
end
for i in 1+length(L):deg
unsafe_store!(addr, i - 1, i)
end
return perm
Copy link
Member Author

@fingolfin fingolfin Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still fails because I only did the "unsafe" version which assumes the input is valid.

But the input could contain duplicates or "out of bounds" entries. I'll keep this one as unsafe_perm or so, and add one that performs those checks, just like the one in the GAP kernel (let's see how much time difference then remains)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lgoettgens lgoettgens lgoettgens left review comments

@ThomasBreuer ThomasBreuer ThomasBreuer approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request optimization Simpler/more performant code or more/better tests package: GAP release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: groups

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fingolfin @ThomasBreuer @lgoettgens